-
Couldn't load subscription status.
- Fork 2
Drop dependencies feedback #191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: invocation-context
Are you sure you want to change the base?
Conversation
This marks a few more methods of ThingServer as private, and accepts a dict of Things to be created during initialisation. In order to do this cleanly, I have now formalised a schema for config files, using a Pydantic model. Tests (and no doubt fixes) will come in the next commit.
The config modules now do all the required validation, including Thing names, ensuring we expand classes into full `ThingConfig` objects, and supplying default values for args, kwargs, etc. I've exposed ThingConfig and ThingServerConfig at module level as they are likely useful in a number of scenarios, particularly writing tests. It is possible to supply a dict whenever a ThingConfig is required - I'll include this in the tests, but I don't want to recommend it - it's better to use the model as it makes it harder to miss fields.
There was duplicated validation code in ThingServer, which I've removed, in favour of using the ThingServerConfig model. I've also split up the code adding Things, so now we: 1. Create Things (and supply a ThingServerInterface) 2. Make connections between Things. 3. Add Things to the API. Step (3) may move out of __init__at some point in the future.
If the server is started from the command line, we print out any validation errors, then exit: a stack trace is not helpful. This also fixes an issue where ValidationError would not serialise properly, which stopped the test code working (it used multiprocessing).
I've removed ThingServer.add_thing in favour of passing a dictionary to ThingServer.__init__ and the test code is updated to reflect that. As a rule, when thing instances were required, I would: 1. Create the server 2. Assign `my_thing = server.things["my_thing"]` 3. Assert `assert isinstance(my_thing, MyThing)` That was enough for `pyright` (and I guess mypy) to correctly infer the type in my test code. It's slightly more verbose, but means we can use the same code in test and production, rather than needing a different way to create/add things in test code.
The name `thing_connection` confused people. `thing_slot` suggests we are marking the attribute and its value will be supplied later, which is exactly right. This commit makes that swap. I've searched/replaced every instance of ThingConnection and thing_connection: we should check the docs build OK, this is perhaps most easily done in CI.
This has been removed as its functionality is provided by `pydantic.ImportString`. It is no longer used for the fallback server: we gain nothing from the dynamic import. In the future, if we want to make it configurable, we could use `ImportString` there too.
Now that it's imported statically, `mypy` spots typing errors with the fallback server. These are now annotated correctly.
I've reworked core concepts and removed DirectThingClient, replacing it with a new "structure" page that I think preserves most of the still-useful content.
b2c9dca to
aa04aff
Compare
I've added an explanation comment with a link to the open pydantic issue.
Barecheck - Code coverage reportTotal: 94.38%Your code coverage diff: 0.13% ▴ Uncovered files and lines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor points
| server = lt.ThingServer( | ||
| { | ||
| "thing_a": ThingA, | ||
| "thing_b": ThingB, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels more clear to explicitly create variable for config so it has meaning.
| server = lt.ThingServer( | |
| { | |
| "thing_a": ThingA, | |
| "thing_b": ThingB, | |
| } | |
| ) | |
| config = {"thing_a": ThingA, "thing_b": ThingB} | |
| server = lt.ThingServer(config) |
| # I've had to type ignore this line because the *args causes an error. | ||
| # Given that *args and **kwargs are very loosely typed anyway, this | ||
| # doesn't lose us much. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem like it is still true?
This PR implements the high level feedback from @julianstirling and @bprobert97 on my series of PRs to address #182 .
It makes two major API changes:
Thinginstances are now created inThingServer.__init__and connected immediately afterwards: this minimises the number of distinct phases in the Thing lifecycle, and avoids confusion in test code where a server might be in a partially initialised state.thing_connectionis renamed tothing_slotand the docs are updated to use language along the lines of "mark this as something the server should supply later"In making the first of these changes, I ended up tidying up the relevant methods in the server, and creating a
pydanticmodel for the server configuration. This simplifies validation of configuration, and also handles loading Thing classes from object reference strings. I've removed my home-made function that did that.While there are a lot of files changed, the majority of these are just changes to how Things are created in test code, and the name change for
thing_slot.